Css 4116/backport crossmodel queries #964

ale8k · 2023-06-22T11:56:47Z

Description

Backports crossmodel queries to v1.

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

Test instructions

Notes for code reviewers

internal/jimm/model_status_parser.go

kian99 · 2023-06-22T13:05:08Z

internal/jimm/model_status_parser.go

+
+		// We use very specific formatting parameters to ensure like-for-like output
+		// with the default juju client installation performing a "status --format json".
+		formatter := status.NewStatusFormatter(*params)


Out of curiosity, why does the NewStatusFormatter constructor require a params object that is itself derived from a model? I would've thought a formatter could be declared once outside this loop and then a call to formatter.Format(modelUUID) would be a more sensible API.

I think they wanted to encapsulate it, but honestly not sure, I would've preferred just a function too honestly

kian99 · 2023-06-22T13:06:17Z

internal/jimm/model_status_parser.go

+		}
+		// We could use output.NewFormatter() from 3.0+ juju/juju, but ultimately
+		// we just want some JSON output, regardless of user formatting. As such json.Marshal
+		// *should* be OK. But TODO: make sure this is fine.


How will we make sure this is fine. This feels like one of those TODOs that will live forever.

You're right, it probably will lol. But it at least leaves some context... I guess... :D

kian99 · 2023-06-22T13:19:51Z

internal/jimm/model_status_parser.go

+//
+// First, call LoadModel, this will retrieve a model from JIMM's database.
+// Next, simply call GetParams.
+type formatterParamsRetriever struct {


This struct and the whole API around it feel a little to easy to use incorrectly. I see what you mean that it's a sort of builder. It's all internal so not really an issue so feel free to ignore this but what do you think about changing the constructor so that it also accepts a model UUID, and does the loadModel and dialModel calls from within the constructor, feel's a little more... cohesive?

Also because currently you can create a formatterParamsRetriever and if you're using the low level primitives loadModel and dialModel and don't do things in the right order, you might forget to run loadModel, then your dialModel looks like its working but is dialling the wrong model.

Happy to come and refactor afterwards but yeah, you could misuse it. This is the current state in feature branch, happy to come back and change though afterwards.

alesstimec · 2023-06-26T10:16:46Z

internal/jimm/jimm.go

 	"github.com/juju/juju/core/crossmodel"
+	jujuparams "github.com/juju/juju/rpc/params"


interesting.. why was this reordered?

Not sure tbh, I directly c/p'd this

internal/jimm/model_status_parser.go

alesstimec · 2023-06-26T10:19:09Z

internal/jimm/model_status_parser.go

+		Errors:  make(map[string][]string),
+	}
+
+	query, err := gojq.Parse(jqQuery)


feels query parsing should be part of the jujuapi package.. i think of it like endpoint handler code vs backend code.. this sort of input validation should happen in the endpoint handler code (jujuapi in this case) and should return a bad request error

I think jujuapi is really an entry point, and jimm is a service level package (at least that's what it appears to be setup like). Such that jujuapi handlers call jimm services? Happy to chat on stdup though?

alesstimec · 2023-06-26T10:23:01Z

internal/jimm/model_status_parser.go

+		return results, errors.E(op, err)
+	}
+
+	// We remove "model:" from the UUIDs, unfortunately that's what OpenFGA returns now after


wait.. what? what model:... uuids should be pure UUIDs not openfga model tags..

In feature-rebac they changed it, it now returns key:value, idk why, but they did in 1.*

alesstimec · 2023-06-26T10:27:02Z

internal/jimm/model_status_parser.go

+// If a result is erroneous, for example, bad data type parsing, the resulting struct field
+// Errors will contain a map from model UUID -> []error. Otherwise, the Results field
+// will contain model UUID -> []Jq result.
+func (j *JIMM) QueryModelsJq(ctx context.Context, modelUUIDs []string, jqQuery string) (params.CrossModelQueryResponse, error) {


this seems a bit inefficient. first we can GetUserModels, which returns all models user has access to, then we extract UUIDs.. only to pass them to a method that fetches those same models from the DB again.. let's just pass models []dbmodel.Model as parameter to this method

Ahhh nice spot

alesstimec · 2023-06-26T10:28:19Z

internal/jimm/model_status_parser.go

+
+	// Set up a formatterParamsRetriever to handle the heavy lifting
+	// of each facade call and type conversion.
+	retriever := newFormatterParamsRetriever(j)


the name is misleading.. as it fetches and formats the model status

It doesn't format the statuses? The formatter.Format() does?

alesstimec · 2023-06-26T10:28:58Z

internal/jimm/model_status_parser.go

+// GetParams retrieves the required parameters for the Juju status formatter from the currently
+// loaded model. See formatterParamsRetriever.LoadModel for more information.
+func (f *formatterParamsRetriever) GetParams(ctx context.Context, modelUUID string) (*status.NewStatusFormatterParams, error) {
+	if err := f.loadModel(ctx, modelUUID); err != nil {


this is what i was talking about.. GetUserModels already loads all models.. and here we are fetching it again from the db

Ya refactored now just testing

Seems like this is a pretty important improvement to bring into feature-rebac as well

alesstimec · 2023-06-26T10:30:09Z

internal/jimm/model_status_parser.go

+}
+
+// storageListAPI acts as a wrapper over our implementation of the juju client, seen in ./internal/jujuclient.
+// This enables us to use storage.GetCombinedStorageInfo without having to c/p the logic we require.


which bits of logic are we copying?

Without this, we'd need to c/p a lot from juju, this was a few months ago but I remember discussing this with you

alesstimec · 2023-06-26T10:32:19Z

internal/jujuapi/jimm.go

@@ -52,6 +54,7 @@ func init() {
 		r.AddMethod("JIMM", 3, "UpdateMigratedModel", updateMigratedModelMethod)
 		r.AddMethod("JIMM", 3, "AddCloudToController", addCloudToControllerMethod)
 		r.AddMethod("JIMM", 3, "RemoveCloudFromController", removeCloudFromControllerMethod)
+		r.AddMethod("JIMM", 3, "CrossModelQuery", crossModelQueryMethod)


would we want this to be JIMM 4? i know we only have the two JIMMs in stg and production, but if i were to use the new jimmctl against those, they both have JIMM facade version 3, but not the new method

I'm not sure, happy to do what you think is best

….com/ale8k/jimm into css-4116/backport-crossmodel-queries

alesstimec

LGTM but i really don't like the formatter naming..

i honestly think that even
thingThatFetchesAndFormatsModelStatus
would be a better name :)

alesstimec · 2023-06-26T12:15:59Z

internal/jimm/model_status_parser.go

+
+	query, err := gojq.Parse(jqQuery)
+	if err != nil {
+		return results, errors.E(op, err)


errors.E(op, "failed to parse jq query", err)

….com/ale8k/jimm into css-4116/backport-crossmodel-queries

ale8k and others added 10 commits June 21, 2023 11:03

Model status parser

96ed7b1

Bring in API definition

9326fd0

Bring in storage facade calls

8b53ebe

Update jimmtest api

8b74e4d

Add api call

d3f8d90

Add API params

83173be

Facade call

6fec97a

Initial backport of parser

3914d64

Fix relation test

dac2853

Merge branch 'main' into css-4116/backport-crossmodel-queries

9e301ba

kian99 approved these changes Jun 22, 2023

View reviewed changes

alesstimec requested changes Jun 26, 2023

View reviewed changes

ale8k added 3 commits June 26, 2023 11:46

PR comments

b536670

Refactor according to PR comments

25f298e

Merge branch 'css-4116/backport-crossmodel-queries' of https://github…

858746a

….com/ale8k/jimm into css-4116/backport-crossmodel-queries

alesstimec approved these changes Jun 26, 2023

View reviewed changes

ale8k and others added 3 commits June 26, 2023 13:54

Merge branch 'main' into css-4116/backport-crossmodel-queries

2e9f1b2

pr comments

6f278e9

Merge branch 'css-4116/backport-crossmodel-queries' of https://github…

63278df

….com/ale8k/jimm into css-4116/backport-crossmodel-queries

ale8k merged commit 46c42ea into canonical:main Jun 26, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Css 4116/backport crossmodel queries #964

Css 4116/backport crossmodel queries #964

ale8k commented Jun 22, 2023

kian99 Jun 22, 2023

ale8k Jun 22, 2023

kian99 Jun 22, 2023

ale8k Jun 22, 2023

kian99 Jun 22, 2023

kian99 Jun 22, 2023

ale8k Jun 22, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

kian99 Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec Jun 26, 2023

ale8k Jun 26, 2023

alesstimec left a comment

alesstimec Jun 26, 2023

		"github.com/juju/juju/core/crossmodel"
		jujuparams "github.com/juju/juju/rpc/params"

Css 4116/backport crossmodel queries #964

Css 4116/backport crossmodel queries #964

Conversation

ale8k commented Jun 22, 2023

Description

Engineering checklist

Test instructions

Notes for code reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alesstimec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment